-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(auth): platform instance ownership in policies #8396
feat(auth): platform instance ownership in policies #8396
Conversation
@@ -371,6 +365,29 @@ private boolean isRoleMatch(final Urn actor, final DataHubActorFilter actorFilte | |||
.anyMatch(actorRoles::contains); | |||
} | |||
|
|||
private Set<String> resolveEntityOwnersForType(Urn entityUrn, List<Urn> ownershipTypes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is the same as getOwnersForType
but renamed to align with the other ones (resolveRoles
or resolveGroups
) and the the first parameter becomes a Urn instead of a ResourceSpec for reusability.
Would really urge you to consider the following approach. Containers + Platform instances are containers, just like Glossary Term Groups. Glossary Term Groups support a privilege called "Manage Children" which grants all access to the children groups + terms. During evaluation time, we check whether there is a parent term group for which the user has "Manage Children" permissions. If yes, we authorize actions against the child groups or terms. I'd recommend a similar approach here to retain consistency. This will also make the default setup experience much simpler for the majority of users, most of which do not actively leverage the platform instance concept today. This will also reduce the scope of this PR. If we continue down the path suggested by this PR, I can see this final panel exploding to support "Container" and "Container owner" use cases and "Domain" and "Domain owner" use cases as well, but I do not feel that this is the right place to declare controls for managing such groups of assets. What do you think? Cheers |
Hi @jjoyce0510 for the feedback and for taking the time to review. I've been discussing with my team the comments here and in our private discussion. Also we have been analysing the code about glossary terms to compare the approach there with the one in this PR. We have some concerns on the approach you are suggesting, considering the information we have at hand and our understanding. So please correct us if we are wrong. The concerns are:
Sorry for the long comment 😅 Just trying to share our point of view. Hopefully we can reach a point that satisfies everybody. The way we see this in the long term future is: Please, let us know if you find a way to unblock this. Looking forward to your response. |
Responding here.
So the idea we took for Glossary Term Groups is to have BOTH a broad "Manage Children" permission, along with more granular permissions: Edit Children Tags, Edit Children Glossary Terms.... Etc, you can imagine listing out all the core entity-level permissions.
Performance cost IS REAL concern. I think that we can of course improve how we do things by fetching container hierarchies, or using the new Browse V2 path that was recently added to make efficient fetching of all parent containers much easier. But I think we'll have to face this cost. Usually real-world container paths are luckily not that deep. Final concern:
We'll indeed need to update some resolvers to basically check whether the current actor has the ability to perform the action based on permissions associated with a parent container.. For example, in the We've actually heard this use case around Containers, Platforms, Platform Instance management before.. and want to make sure that we make it as generic as possible.
This is CORRECT. GraphQL layer handles app-layer authorization. The current assumption is that the Ingestion Clients have full access to change the metadata graph. Does this address the concerns? If I were implementing the feature today, this is how I'd build it! |
I find this very redundant... from the point of view of the user as well as the implementation such approach requires.
I'm ok to rely on such assumption.
TBH this was new for us and shocking 😅 We need to investigate the capabilities the new
We talk about the "Manage Children" permission, but actually, this hierarchy is more semantic than properly structured in the entity graph. Eg. Platform Instance is 1:1 relationship with Dataset and Container... but is not reachable from the Container hierarchy itself. The same for Platform. The same for Domain... is domain above or below a Platform or a Platform Instance? And we also have the new Data Products. Unless I'm missing something, the overall hierarchy is not so clear, much more closer to a graph and then we can have loops 😅 |
@jjoyce0510 As discussed yesterday:
Of course, in parallel we can have discussions on how to eventually fix this in the upstream project 👍 @amanda-her You can close this PR once you are back 😄 |
…erhip_in_policies
Closing per previous comment. |
This PR introduces two fields for DataHubPolicyInfo:
platformInstanceOwners
andplatformInstanceOwnersTypes
. With this change authorization policies can be restricted to platform instance owners in case a Metadata asset belongs to a particular platform instance. By defaultplatformInstanceOwners
is set to false. When this flag is set to True, it is possible to specify which particular types (i.e.TECHNICAL_OWNER
,BUSINESS_OWNER
) of ownership will be considered for the policy with theplatformInstanceOwnersTypes
field. As a result, for example, the option of granting rights to change tags of datasets to only the owners of the platform instance they belong to is available. In addition, the type of this ownership can be specified.Beside changes in the
PolicyEngine
and other parts of backend, also UI was amended to allow for new capability to be set as show below:Checklist
Authorization policies restricted to platform instance owners
It would be nice to update documentation in access-policies-guide.md, but images must be uploaded to separate repo.